Skip to content

Harmonize past_key_value to past_key_valueS everywhere - #39956

Merged
Cyrilvallez merged 14 commits into
mainfrom
past-kv
Aug 8, 2025
Merged

Cyrilvallez merged 14 commits into
mainfrom
past-kv

Conversation

@Cyrilvallez

@Cyrilvallez Cyrilvallez commented Aug 6, 2025 •

Copy link
Copy Markdown
Member

As per the title. I'm getting annoyed to see both, it was time to finally make it coherent everywhere.
All the changes are made, so I'll only need to remove the decorators after next release (it's only internal modules, so no need for a long deprecation cycle)

Also reapplied modular to examples/modular-transformer

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Cyrilvallez Cyrilvallez changed the title Harmonize past_key_value to past_key_values everywhere Harmonize past_key_value to past_key_valueS everywhere Aug 6, 2025

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking! Let's add just 1 deprecation cycle, it does not cost much IMO!

Comment on lines -73 to -77
if "past_key_value" in kwargs and kwargs["past_key_value"] is not None:
kwargs["past_key_value"] = None
message += " `past_key_value=None`,"
do_warn = True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to not break we still nedd this no?

@Cyrilvallez Cyrilvallez Aug 8, 2025 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm good catch, since the decorator will change the name correctly on the forward args but this is __call__ of the Module, I believe it should indeed still be there!

@github-actions

github-actions Bot commented Aug 8, 2025

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: arcee, aria, autoformer, bamba, bart, bert, bert_generation, big_bird, bigbird_pegasus, biogpt, bitnet, blenderbot, blenderbot_small

@Cyrilvallez
Cyrilvallez merged commit 5c3fb7f into main Aug 8, 2025
@Cyrilvallez
Cyrilvallez deleted the past-kv branch August 8, 2025 09:53
ydshieh added a commit that referenced this pull request Aug 16, 2026
…h WhisperForCausalLM

Fix 1 — is_updated UnboundLocalError (WhisperAttention.forward)

PR #39956 wrapped the `is_updated` assignment in an isinstance guard but left it
consumed unconditionally below. The bug was dormant while Whisper always received
EncoderDecoderCache. PR #43679 changed generate() to pre-initialize a plain DynamicCache
for is_encoder_decoder=False models, activating the latent bug. Fix: initialize
is_updated=False before the guard, identical to PR #40517 which fixed 55 other models.

Fix 2a — Wrong cache type: DynamicCache instead of EncoderDecoderCache (WhisperDecoder.forward)

generate() uses config.is_encoder_decoder to pick the cache type. WhisperForCausalLM
sets is_encoder_decoder=False, so PR #43679 made generate() pre-initialize a plain
DynamicCache. But WhisperForCausalLM uses cross-attention: self- and cross-attention
share the same layer_idx slots in DynamicCache and overwrite each other. Fix: when
generate() passes an empty DynamicCache and encoder_hidden_states are present, convert
it to EncoderDecoderCache before the first forward pass.

Fix 2b — Wrong layer count: 32 slots pre-allocated for a 2-layer decoder (WhisperDecoder.forward)

DynamicCache(config=self.config) calls config.get_text_config(decoder=True), which for
WhisperConfig returns self unchanged (flat config, no nested decoder sub-config). It then
reads config.num_hidden_layers, which WhisperConfig maps via attribute_map to
encoder_layers=32. For distil-whisper-large-v2, encoder_layers=32 but decoder_layers=2,
so 32 cache slots are pre-allocated for a 2-layer decoder. Slots 2-31 are uninitialized
and crash on crop(-n). Fix: deepcopy the config and override num_hidden_layers with
decoder_layers before passing to DynamicCache.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ydshieh added a commit that referenced this pull request Aug 16, 2026
…on, and speed regression (#48000)

* [Whisper] Fix cache type and layer count for speculative decoding with WhisperForCausalLM

Fix 1 — is_updated UnboundLocalError (WhisperAttention.forward)

PR #39956 wrapped the `is_updated` assignment in an isinstance guard but left it
consumed unconditionally below. The bug was dormant while Whisper always received
EncoderDecoderCache. PR #43679 changed generate() to pre-initialize a plain DynamicCache
for is_encoder_decoder=False models, activating the latent bug. Fix: initialize
is_updated=False before the guard, identical to PR #40517 which fixed 55 other models.

Fix 2a — Wrong cache type: DynamicCache instead of EncoderDecoderCache (WhisperDecoder.forward)

generate() uses config.is_encoder_decoder to pick the cache type. WhisperForCausalLM
sets is_encoder_decoder=False, so PR #43679 made generate() pre-initialize a plain
DynamicCache. But WhisperForCausalLM uses cross-attention: self- and cross-attention
share the same layer_idx slots in DynamicCache and overwrite each other. Fix: when
generate() passes an empty DynamicCache and encoder_hidden_states are present, convert
it to EncoderDecoderCache before the first forward pass.

Fix 2b — Wrong layer count: 32 slots pre-allocated for a 2-layer decoder (WhisperDecoder.forward)

DynamicCache(config=self.config) calls config.get_text_config(decoder=True), which for
WhisperConfig returns self unchanged (flat config, no nested decoder sub-config). It then
reads config.num_hidden_layers, which WhisperConfig maps via attribute_map to
encoder_layers=32. For distil-whisper-large-v2, encoder_layers=32 but decoder_layers=2,
so 32 cache slots are pre-allocated for a 2-layer decoder. Slots 2-31 are uninitialized
and crash on crop(-n). Fix: deepcopy the config and override num_hidden_layers with
decoder_layers before passing to DynamicCache.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* [Whisper] Restore absolute cache crop in _update_past_and_masks for speculative decoding

Fix 3 — Cache size accumulation (candidate_generator.py)

Commit 5ec7b73 ("crop with negative always") changed _update_past_and_masks to always
remove a fixed number of tokens (crop(-1)) instead of cropping to an absolute target.
When multiple speculative candidates are rejected in a round, the cache grows by several
tokens but only 1 is trimmed. Over successive rounds, cache.get_seq_length() exceeds
input_ids.shape[1]. _prefill then computes next_sequence_length = input_ids.shape[1] -
cache.get_seq_length() which goes negative, causing input_ids[:, -n:] to produce a [1, 0]
empty tensor that crashes in WhisperAttention.forward() at the reshape.

Verified by bisect: at the parent of 5ec7b73 (68075f8), fixes 1+2a+2b alone are
sufficient (speed assertion only). At 5ec7b73 with only fixes 1+2a+2b the reshape
crash reproduces. Adding this fix restores the test to speed-assertion-only.

Fix: compute the absolute target size from input_ids and only crop if the cache exceeds
it, using a negative delta to comply with the current Cache.crop(negative) API.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* [Generate] Sync adaptive confidence threshold to generation_config for ConfidenceCriteria

PR #42702 introduced `self.assistant_generation_config` (a deepcopy with defaults applied)
to hold the assistant model's config, but the adaptive threshold update
(`update_candidate_strategy`) was only syncing the new best threshold to
`self.assistant_generation_config.assistant_confidence_threshold`, not to
`self.generation_config.assistant_confidence_threshold`.

`ConfidenceCriteria` is constructed each round from `self.generation_config`, so it was
always using the initial 0.4 threshold regardless of what the ROC-curve adaptation computed.
This caused the assistant to always generate the maximum number of speculative tokens per
round, making assisted generation slower than non-assisted on the distil-whisper test.

Fix: also update `self.generation_config.assistant_confidence_threshold` whenever the
adaptive threshold is updated, so each new round's ConfidenceCriteria reflects the current
best threshold.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* [Whisper] Format: wrap long EncoderDecoderCache line

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* [AudioFlamingo3] Fix is_updated UnboundLocalError in cross-attention (sync with modular)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants